[core][python] Fix global index coverage for residual predicates - #9050
Conversation
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
I left one correctness comment and one non-blocking API naming suggestion.
| for group in groups: | ||
| pad_ranges = _exclude_ranges(union_coverage, group.coverage_ranges) | ||
| readers.extend( | ||
| _create_readers( |
There was a problem hiding this comment.
[P1] This merge can let an unsupported alternate index poison an otherwise usable primary reader. For example, if c has a dedicated BTree index and is also a companion/extra field of a Java-built multi-column es-index(a, c), the base branch returns the BTree result, but this loop also instantiates the es-index group and _create_inner_readers raises ValueError because PyPaimon does not support that index type. FileScanner silently loses pruning, while the indexed and raw vector pre-filter paths propagate the exception and fail the query. Please make alternate selection capability-aware while keeping coverage conservative: either exclude unreadable coverage or represent those ranges as all-hit/fallback padding, and add a regression test with a supported primary plus a real unsupported extra-field index.
There was a problem hiding this comment.
[P1] This merge can let an unsupported alternate index poison an otherwise usable primary reader. For example, if
chas a dedicated BTree index and is also a companion/extra field of a Java-built multi-columnes-index(a, c), the base branch returns the BTree result, but this loop also instantiates thees-indexgroup and_create_inner_readersraisesValueErrorbecause PyPaimon does not support that index type.FileScannersilently loses pruning, while the indexed and raw vector pre-filter paths propagate the exception and fail the query. Please make alternate selection capability-aware while keeping coverage conservative: either exclude unreadable coverage or represent those ranges as all-hit/fallback padding, and add a regression test with a supported primary plus a real unsupported extra-field index.
fixed
There was a problem hiding this comment.
Verified this on my side. Reproduced with a dedicated BTree index on c over [0,4] plus an es-index(a, c) extra-field group over [5,9]: the base branch returned the BTree result, while the original commits raised ValueError from _create_inner_readers (silent full-scan fallback in FileScanner._eval_global_index, hard failure in the vector raw pre-filter). The capability-aware filtering in feb8c8a fixes it — the repro returns the BTree result again, and since the unreadable files also drop out of _coverage, ranges [5,9] correctly become unindexed fallback instead of being claimed as indexed. The new regression test asserts the right contract.
| public static final class Evaluation { | ||
|
|
||
| private final GlobalIndexResult result; | ||
| private final Set<Integer> fieldIds; |
There was a problem hiding this comment.
Non-blocking: could we rename fieldIds to contributingFieldIds (and keep the Python field/method names in sync)? This set intentionally excludes unsupported or discarded branches; it is not the full predicate field set or every field that was evaluated. The provenance distinction is the core contract of this fix, and these APIs are introduced in this PR, so naming it explicitly now would make future coverage call sites much harder to misuse. Suggested contract: “Field IDs whose supported index results were combined into the returned candidate; unsupported or discarded branches are excluded.”
There was a problem hiding this comment.
Non-blocking: could we rename
fieldIdstocontributingFieldIds(and keep the Python field/method names in sync)? This set intentionally excludes unsupported or discarded branches; it is not the full predicate field set or every field that was evaluated. The provenance distinction is the core contract of this fix, and these APIs are introduced in this PR, so naming it explicitly now would make future coverage call sites much harder to misuse. Suggested contract: “Field IDs whose supported index results were combined into the returned candidate; unsupported or discarded branches are excluded.”
Updated
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Reviewed the full PR including the two new commits (feb8c8a, 277e4e3). Verified locally at head 277e4e3:
GlobalIndexEvaluatorTest: 28/28 passedBtreeGlobalIndexTableTest: 19/19 passed (incl. the new full-mode coverage test)- PyPaimon global-index + vector suites: 182 passed, 5 skipped
P1 verification (before the fix). I reproduced JingsongLi's scenario with a minimal script: field c with a dedicated BTree index over [0,4] plus an unsupported es-index(a, c) carrying c as an extra field over [5,9], query c = 42. On the base branch the scan returns the BTree result; on the first 4 commits it raised ValueError: Unsupported global-index type in scanner: 'es-index' — FileScanner._eval_global_index would swallow that into a silent full scan, and _raw_pre_filter would propagate it and fail the query. So the concern was real.
The fix looks right. Filtering unsupported scalar index files at the scanner boundary (constructor + both create paths + index_file_filter) is the clean solution: the unreadable ranges drop out of _coverage as well, so they become unindexed fallback ranges instead of being silently claimed as indexed — coverage stays conservative and no rows are dropped. My repro now returns the BTree result again, and the new test_unsupported_extra_field_index_does_not_poison_primary asserts exactly the right contract (only btree instantiated, fallback = Range(5, 9)).
Semantics check on contributing-field tracking. The AND/OR bookkeeping is sound: OR unions child field IDs (a row is trusted only where every branch's fields have index answers, which matches the intersection-based coverage), and the AND early-break keeping only fields processed so far is also safe — once the compounded candidate is empty, rows covered by those fields are proven non-matching regardless of remaining fields, so excluding them from the residual fallback is correct. Keeping the fallback as List[Range] (scalar planning and now the vector raw pre-filter) avoids materializing huge FULL-mode bitmaps. The Python dedicated+extra union with padding is equivalent to Java's UnionGlobalIndexReader for exact indexes.
Two minor nits, non-blocking:
DataEvolutionGlobalIndexScanner.unindexedRows(Predicate)(Java) now has no production callers and keeps the old all-predicate-fields coverage semantics — the exact misuse this PR fixes. Suggest deleting it so nobody reintroduces the bug; the Python predicate-based fallback branch inunindexed_rows/unindexed_ranges(whencontributing_field_ids=None) is in the same situation._SUPPORTED_SCALAR_INDEX_TYPEShardcodes'full-text'; consider reusingFULL_TEXT_IDENTIFIERfrompypaimon.globalindex.full_textif that doesn't create an import cycle, to avoid drift.
LGTM once CI is green.
|
+1 |
3 participants
Purpose
For an
ANDpredicate containing an indexed field and an unindexed residual field, global-index evaluation uses the indexed candidate correctly. However, coverage was calculated from the complete predicate, sofullsearch mode treated the indexed range as uncovered and expanded the result to a full scan.Track the field IDs whose index results actually contribute to predicate evaluation and use only those fields for coverage. Apply the fix to Java and PyPaimon scalar scans and raw vector pre-filtering. Unsupported
ORbranches remain residual filters and do not contribute field IDs.Also make PyPaimon read a field dedicated primary index together with indexes carrying it as an extra field. Coverage already included both sources; ignoring the extra-field reader could otherwise miss rows. Preserve the single-index fast path.
For scalar planning, keep uncovered row ranges as
List[Range]. Only convert indexed matches from the bitmap, then merge both range lists. This avoids iterating every row ID in a largefullfallback bitmap.Tests
GlobalIndexEvaluatorTest: 28 passedBtreeGlobalIndexTableTest#testFullSearchIgnoresUnindexedAndResidualForCoverage: passedpy_compile, flake8, Spotless, andgit diff --check: passed